-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pre-bundle to speed up swingset unit tests #1644
Conversation
Using `rollup` to turn a tree of vat-defining source files into a bundle object is somewhat slow. It seems to depend on the amount of code being bundled: the worst case is the kernel sources, which take 1.5s . The kernel needs a number of other vats which are automatically bundled at runtime. The total process takes about 2.7s on my machine. We have unit tests that run many swingsets in a single test file. The most extreme case is test-message-patterns.js, which calls `buildVatController` 60 times in a row. It takes a really long time to run, 3-6 minutes, and the majority is the bundling step. Parallelizing this doesn't help (JS is a single-threaded language, it's not using non-blocking filesystem operations, I don't know whether it's CPU or IO bound but either would be a problem). This patch exports a new `buildKernelBundles()` function to let these multi-swingset tests build the bundles once per test file. It also adds `kernelBundles` to the `buildVatController(.., .., runtimeOptions)` options bag to pass these bundles back in (with sensible defaults). The controller gets all the built-in bundles from this collection, rather than running `bundleSource` each time. It also rearranges the console setup slightly, to make the `console` object available earlier within `buildVatController`. I needed this to debug problems with unpacking the other `runtimeOptions`. Also, now that we're building our own `console` object, we no longer need to explicitly harden the native console's (unique) prototype object. refs #1643
To support `buildVatController(.., argv)`, the code modified the `config` object (replacing `.parameters` on the bootstrap vat with the argv contents). This mutation of the inbound `config` argument is a bit surprising, and interferes with multi-swingset unit tests that might want to share the config object among multiple `buildVatController` calls, or if the `config` object is hardened. This patch makes the controller build a new `config` object out of the pieces of the original, with the new parameters overlaid on top. Provides most of the fix to #1490 , but doesn't close it, since we still have to think about `.devices`.
This changes test-message-patterns.js to build all vat bundles ahead of time, once, in a `test.before()` clause, and shares both the bundles and the config objects among all tests. This speeds up the overall test from 230s to 12s on my machine. It also removes lingering support for enabling/disabling pipelining, which I needed when first implementing pipelining, but is now just in the way. And a few noisy console.logs are gone. We can achieve further speedups by amortizing bundling of the loopbox device code, once the swingset config can accept device bundles in addition to sourcePaths.
This now takes 6s, vs 22s before. We can probably speed it up further by pre-bundling the test-specific vat sources too.
This now takes 6s, vs 26s before.
This now takes 6s, vs 16s before. I also removed the `copy(config)`, now that `buildVatController` no longer mutates it. We can probably speed it up further by pre-bundling the test-specific vat sources too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Surprisingly non-invasive.
@@ -16,6 +16,11 @@ function capargs(args, slots = []) { | |||
return capdata(JSON.stringify(args), slots); | |||
} | |||
|
|||
test.before(async t => { | |||
const kernelBundles = await buildKernelBundles(); | |||
t.context.data = { kernelBundles }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is t.context
something provided by Ava or are we just doing this and hoping it will be safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comms: require.resolve('./vats/comms'), | ||
vattp: require.resolve('./vats/vat-tp'), | ||
timer: require.resolve('./vats/vat-timerWrapper'), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not obviously a use case we have an immediate need to support, but it strikes me that we've got most of the building blocks here to allow kernels to be built without some of this stuff in cases where the swingset being launched doesn't need them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, #1351 is an opportunity to set up devices a bit more coherently, in which case we could defer adding the timer vat until someone gives us a timer device to work with. OTOH #1346 might manage to make devices sufficiently vat-like that we don't need wrappers anymore and that one goes away.
I'm inclined to think that comms is a sufficiently core part of the swingset experience ("what's life without IO?") that it's probably always going to be there, but yeah, who knows.
This adds a
kernelBundles
option tobuildVatController
, abuildKernelBundles
function to populate it, and converts the four largest swingset unit tests to use it.It reduces the
cd packages/SwingSet; yarn test
time from 307s to 78s on my machine, a 4x speedup.@FUDCo you might want to review the first two commits separately.
refs #1643
refs #1490